-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test(app-project): run YourStats tests in a non-UTC timezone #6412
Conversation
4fdec87
to
52802f9
Compare
I read through the code and approved, but then I ran the tests locally on my local machine which is set to US Central Time. These tests don't pass |
You can test this by changing your local machine's time settings. I just set mine to Monday Oct 28 UTC and the tests pass locally. They do not pass when mine is set to today's date and Chicago time. |
Yep, these tests catch the bug that you found in #2244, which doesn't trigger when you run the code in a UTC time zone. |
See #2244 (comment). Reviewing this PR is on hold until design decisions are made about YourStats. |
@goplayoutside3 it works now when it's run in CDT, but there doesn't seem to be an easy way to check that in CI. EDIT: This version passes in Guam, Kuala Lumpur, London, and Chicago. |
@@ -74,7 +80,7 @@ const YourStats = types | |||
weekDay.setUTCDate(newDate) | |||
const period = weekDay.toISOString().substring(0, 10) | |||
const { count } = dailyCounts.find(count => count.period.startsWith(period)) || { count: 0, period } | |||
const dayNumber = weekDay.getDay() | |||
const dayNumber = weekDay.getUTCDay() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was causing the weekday bug in #2244, but it only triggers when you run getDay()
in a time zone other than UTC. Otherwise, getDay()
is identical to getUTCDay()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #2528 for some context on why the code is getting the weekday here.
772623b
to
1bd3d43
Compare
clock = sinon.useFakeTimers({ now: new Date(2019, 9, 1, 12), toFake: ['Date'] }) | ||
// Set the local clock to 1am on Tuesday 1 October 2019, UTC. | ||
// Stats should be shown for Monday 30 September 2019, local time. | ||
clock = sinon.useFakeTimers(new Date('2019-09-30T20:00:00-05:00')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't set the clock timezone to -05:00
. new Date()
always runs in the time zone of the machine that's running the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Methods like date.getDay()
always return values in the OS time zone too.
This is useful. How to run Node tests in different time zones, by setting the https://www.stefanjudis.com/today-i-learned/set-the-default-time-zone-in-node-js/ |
b5f0770
to
55d4d50
Compare
I've updated this to catch #2244 by running the tests in the I've then updated the code to pass the tests. Test-driven development FTW! |
{ count: 12, period: '2019-09-29T00:00:00.000Z' }, | ||
{ count: 12, period: '2019-09-30T00:00:00.000Z' }, | ||
{ count: 13, period: '2019-10-01T00:00:00.000Z' }, | ||
{ count: 14, period: '2019-10-02T00:00:00.000Z' }, | ||
{ count: 10, period: '2019-10-03T00:00:00.000Z' }, | ||
{ count: 11, period: '2019-10-04T00:00:00.000Z' }, | ||
{ count: 8, period: '2019-10-05T00:00:00.000Z' }, | ||
{ count: 15, period: '2019-10-06T00:00:00.000Z' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These mocks should match the API responses for real ERAS data, which uses midnight UTC to mark the start of each day.
@@ -17,7 +17,7 @@ function DailyClassificationsChartContainer({ | |||
const [year, monthIndex, date] = period.split('-') | |||
const utcDay = Date.UTC(year, monthIndex - 1, date) | |||
const day = new Date(utcDay) | |||
const isToday = day.getUTCDay() === TODAY.getDay() | |||
const isToday = day.getUTCDay() === TODAY.getUTCDay() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Today" is always the current day in Greenwich.
A couple of small refactors for the `YourStats` tests: - add missing timestamps to the mock ERAS data, to match real ERAS responses. - test the weekly stats in the `-05:00` timezone, to check for timezone bugs.
15031c7
to
2ba3612
Compare
Just a heads up that this is superseded by #6472 |
Superseded by #6472 |
A couple of small refactors for the
YourStats
tests:test the weekly stats in thethis doesn’t work. Tests always run in the OS time zone.-05:00
timezone, to check for timezone bugs.-06:00:00
timezone by setting the NodeTZ
env var, to catch Daily count stats bug #2244.getUTCDay
to get the day number for daily stats, fixing Daily count stats bug #2244.Please request review from
@zooniverse/frontend
team or an individual member of that team.Package
Linked Issue and/or Talk Post
How to Review
I'm not seeing any errors when I run this, but I'm running it on a computer that's set to UTC. I think it might need to be run in a different time zone, in order to test it. Whatever timezone it's run in, the stats chart should always start on Monday.
Checklist
PR Creator - Please cater the checklist to fit the review needed for your code changes.
PR Reviewer - Use the checklist during your review. Each point should be checkmarked or discussed before PR approval.
General
yarn panic && yarn bootstrap
ordocker-compose up --build
and FEM works as expected